Skip to content

Repurpose CloudConnectorAnnounceTask for daily Sources registration#1214

Open
jeremylenz wants to merge 10 commits into
theforeman:developfrom
jeremylenz:cloud-connector-sources-announce
Open

Repurpose CloudConnectorAnnounceTask for daily Sources registration#1214
jeremylenz wants to merge 10 commits into
theforeman:developfrom
jeremylenz:cloud-connector-sources-announce

Conversation

@jeremylenz

@jeremylenz jeremylenz commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR addresses SAT-45966 / SAT-44641 by repurposing the CloudConnectorAnnounceTask from a REX subscription into a daily recurring task with an API endpoint for immediate registration.

Before (Old Flow — Broken with foremanctl)

  1. User clicks "Configure Cloud Connector" in the Foreman UI
  2. This triggers a Remote Execution (REX) job (ansible_configure_cloud_connector)
  3. When the REX job completes → CloudConnectorAnnounceTask fires (subscribed to RunHostsJob)
  4. Sources registration happens automatically via CloudPresence.announce_to_sources

With cloud connector setup moving to foremanctl, the REX job never runs, so CloudConnectorAnnounceTask never fires and Sources registration never happens.

After (New Flow — Fixed)

  1. User runs foremanctl deploy --add-feature cloud-connector
  2. Immediate registration: foremanctl calls POST /api/v2/rh_cloud/announce_to_sources after setting rhc_instance_id
  3. Daily self-healing: A recurring task checks if registration is needed for each org, with a staggered 0–3 hour delay
  4. Smart skipping: Orgs with recent cloud remediation jobs (past 24h) skip the API call since connectivity is already proven

Changes

  • Converts CloudConnectorAnnounceTask from a REX subscription to a daily RecurringAction + one-shot API trigger
  • Adds POST /api/v2/rh_cloud/announce_to_sources endpoint for immediate registration during foremanctl deploy
  • Adds rhc_connection_exists? check in CloudPresence to avoid duplicate connection errors (Sources API returns 400)
  • Skips Sources API call for orgs with recent cloud remediation jobs
  • Removes the old REX-based cloud connector flow: CloudConnector service class, enable_cloud_connector endpoints, ansible_configure_cloud_connector REX feature
  • Task output distinguishes: Registered / Already registered / Already registered (recent cloud remediation) / Skipped (no manifest) / Failed

Test plan

  • Daily scheduled task runs and registers orgs with valid manifests
  • POST /api/v2/rh_cloud/announce_to_sources triggers immediate registration (no delay)
  • Task skips when rhc_instance_id is not set, in IoP mode, or org has no manifest
  • Task skips Sources API call when recent cloud remediation jobs exist
  • Task shows warning (not success) when any org fails
  • Old REX-based cloud connector functionality is cleanly removed
  • 543 tests pass, 0 failures

🤖 Generated with Claude Code

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • CloudConnectorAnnounceTask#run logs a per-org info line for every organization skipped due to missing manifest; if you expect many orgs this could be noisy, so consider summarizing skipped orgs at the end (similar to output[:status]) instead of logging each one individually.
  • The warning about allow_auto_inventory_upload being disabled in CloudConnectorAnnounceTask#plan will be emitted on every daily run while the setting is off; if that’s an expected long-term configuration for some deployments, consider downgrading to info or adding a guard so the warning is not continuously repeated.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- CloudConnectorAnnounceTask#run logs a per-org info line for every organization skipped due to missing manifest; if you expect many orgs this could be noisy, so consider summarizing skipped orgs at the end (similar to `output[:status]`) instead of logging each one individually.
- The warning about `allow_auto_inventory_upload` being disabled in CloudConnectorAnnounceTask#plan will be emitted on every daily run while the setting is off; if that’s an expected long-term configuration for some deployments, consider downgrading to info or adding a guard so the warning is not continuously repeated.

## Individual Comments

### Comment 1
<location path="lib/insights_cloud/async/cloud_connector_announce_task.rb" line_range="33-46" />
<code_context>
       end

-      def finalize
+      def run
+        announced = []
+        skipped = []
+        failed = []
+
         Organization.unscoped.each do |org|
+          unless cert_auth_available?(org)
+            logger.info("Skipping Sources announcement for organization #{org.name}: no manifest available")
+            skipped << org.name
+            next
+          end
+
           presence = ForemanRhCloud::CloudPresence.new(org, logger)
</code_context>
<issue_to_address>
**suggestion:** Exception handling in `run` drops stack traces, which can complicate debugging

In the `rescue StandardError => ex` block, only `"Failed to announce...: #{ex}"` is logged, so the backtrace is lost. Please also log `ex.full_message` or `ex.message` together with `ex.backtrace.join("\n")` (possibly at debug level or in a separate line) so failures remain diagnosable in production.

```suggestion
        Organization.unscoped.each do |org|
          unless cert_auth_available?(org)
            logger.info("Skipping Sources announcement for organization #{org.name}: no manifest available")
            skipped << org.name
            next
          end

          presence = ForemanRhCloud::CloudPresence.new(org, logger)
          presence.announce_to_sources
          announced << org.name
        rescue StandardError => ex
          logger.warn("Failed to announce to Sources for organization #{org.name}: #{ex}")
          logger.debug(
            "Sources announcement error details for organization #{org.name}: " \
            "#{ex.class}: #{ex.message}\n#{Array(ex.backtrace).join("\n")}"
          )
          failed << org.name
        end
```
</issue_to_address>

### Comment 2
<location path="test/jobs/cloud_connector_announce_task_test.rb" line_range="11-20" />
<code_context>
+    ForemanRhCloud.unstub(:with_iop_smart_proxy?)
+  end
+
+  test 'announces to sources for all organizations when rhc_instance_id is set' do
+    Setting[:rhc_instance_id] = 'test-rhc-id'
+
+    InsightsCloud::Async::CloudConnectorAnnounceTask.any_instance
+      .stubs(:cert_auth_available?).returns(true)

-    @job_invocation = generate_job_invocation(:ansible_configure_cloud_connector)
+    ForemanRhCloud::CloudPresence.any_instance
+      .expects(:announce_to_sources)
+      .times(Organization.unscoped.count)

-    # reset connector feature ID cache
-    InsightsCloud::Async::CloudConnectorAnnounceTask.instance_variable_set(:@connector_feature_id, nil)
+    action = create_and_plan_action(InsightsCloud::Async::CloudConnectorAnnounceTask)
+    run_action(action)
   end

</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions for the task output status to cover the new reporting behavior in `run`

The new `output[:status]` summary (with announced/skipped/failed orgs) isn’t validated by the tests. Please add at least one test that inspects `action.output[:status]` after `run_action(action)` and covers cases like:

- all orgs announced (only `Announced:` present)
- orgs skipped for missing manifest (`Skipped (no manifest):` present)
- orgs failing (`Failed:` present)

This will exercise the new reporting behavior and guard against regressions in the status formatting and content.

Suggested implementation:

```ruby
  test 'announces to sources for all organizations when rhc_instance_id is set' do
    Setting[:rhc_instance_id] = 'test-rhc-id'

    InsightsCloud::Async::CloudConnectorAnnounceTask.any_instance
      .stubs(:cert_auth_available?).returns(true)

    ForemanRhCloud::CloudPresence.any_instance
      .expects(:announce_to_sources)
      .times(Organization.unscoped.count)

    action = create_and_plan_action(InsightsCloud::Async::CloudConnectorAnnounceTask)
    run_action(action)

    # Validate reporting in action output
    assert_equal true, action.output[:success]
    assert_includes action.output[:status], 'Announced:'
    refute_includes action.output[:status], 'Skipped (no manifest):'
    refute_includes action.output[:status], 'Failed:'
  end

```

To fully match the review suggestion, you can add two more tests that exercise the “skipped (no manifest)” and “failed” paths by:
1. Creating organizations that will trigger those branches in `run` (e.g. by stubbing/mocking the parts of `CloudConnectorAnnounceTask` / `CloudPresence` that decide an org is skipped vs failed).
2. Running `run_action(action)` and asserting that `action.output[:status]` includes the respective `Skipped (no manifest):` and `Failed:` fragments (and that the other fragments are absent or have the expected counts).
The exact stubs/exceptions depend on how the implementation distinguishes skipped vs failed orgs in your codebase.
</issue_to_address>

### Comment 3
<location path="test/jobs/cloud_connector_announce_task_test.rb" line_range="59-22" />
<code_context>

-  def read_jsonl(jsonl)
-    jsonl.lines.map { |l| JSON.parse(l) }
+  test 'skips organizations without a manifest' do
+    Setting[:rhc_instance_id] = 'test-rhc-id'
+
+    InsightsCloud::Async::CloudConnectorAnnounceTask.any_instance
+      .stubs(:cert_auth_available?).returns(false)
+
+    ForemanRhCloud::CloudPresence.any_instance
+      .expects(:announce_to_sources)
+      .never
+
+    action = create_and_plan_action(InsightsCloud::Async::CloudConnectorAnnounceTask)
+    run_action(action)
+  end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test that covers behavior when automatic inventory upload is disabled

The `plan` method now warns when `Setting[:allow_auto_inventory_upload]` is false while `rhc_instance_id` is set, but still calls `plan_self`. Current tests don’t distinguish between the true/false cases for `allow_auto_inventory_upload`. Please add a test that sets `Setting[:rhc_instance_id]` with `Setting[:allow_auto_inventory_upload] = false` and verifies the task still runs (e.g., `run_action(action)` leads to `announce_to_sources` being called). This guards against future changes that might skip the task when auto-upload is disabled.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread lib/insights_cloud/async/cloud_connector_announce_task.rb
Comment thread test/jobs/cloud_connector_announce_task_test.rb
Comment thread test/jobs/cloud_connector_announce_task_test.rb Outdated
@jeremylenz jeremylenz force-pushed the cloud-connector-sources-announce branch from d130135 to 1b9de5a Compare June 12, 2026 19:04
@parthaa

parthaa commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Recommend adding something like what the flow used to look like and what changes after your PR

What This PR Does:
Before (Old Flow - Broken):

  1. User runs Cloud Connector setup via UI
  2. This triggers a Remote Execution (REX) job
  3. When REX job completes → CloudConnectorAnnounceTask fires
  4. Sources registration happens automatically

After (New Flow - Fixed):

  1. User runs foremanctl deployment with cloud connector setup
  2. Immediate registration: foremanctl can call new API endpoint POST /api/v2/rh_cloud/announce_to_sources
  3. Backup registration: Daily recurring task checks if registration is needed
  4. Sources registration happens reliably

@jeremylenz jeremylenz force-pushed the cloud-connector-sources-announce branch from 05a3462 to 463b152 Compare June 12, 2026 21:00
Comment thread lib/insights_cloud/async/cloud_connector_announce_task.rb
jeremylenz and others added 7 commits June 12, 2026 17:13
With cloud connector setup moving to foremanctl, the old REX-triggered
Sources registration flow no longer fires. This converts
CloudConnectorAnnounceTask from a REX subscription into a daily
recurring task that checks if rhc_instance_id is set and registers
with Red Hat Sources for each organization with a valid manifest.

Also adds POST /api/v2/rh_cloud/announce_to_sources for immediate
registration during foremanctl deploy, and removes the old REX-based
cloud connector flow (CloudConnector service, enable_cloud_connector
endpoints, ansible_configure_cloud_connector feature).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Downgrade allow_auto_inventory_upload message to debug (avoids noisy
  daily logs for deployments that intentionally disable auto upload)
- Remove per-org skip logging, rely on output[:status] summary instead
- Log backtrace at debug level on per-org failures for diagnosability
- Add output[:status] assertions to tests
- Add test for allow_auto_inventory_upload=false (task still runs)
- Fix rubocop indentation offenses

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Include error details in output[:status] for failed orgs
- Call error! when any org fails so task shows as warning, not success
- Downgrade allow_auto_inventory_upload message to debug
- Remove per-org skip logging, rely on output[:status] summary instead
- Log backtrace at debug level on per-org failures
- Add output[:status] assertions to tests
- Add test for allow_auto_inventory_upload=false (task still runs)
- Fix rubocop indentation offenses

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Sources API returns 400 if an RHC connection already exists for
the given rhc_id. Query for an existing connection first and skip
the POST if one is found, following the same pattern used by
satellite_instance_source for source get-or-create.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use DelayedStart to add a random 0-3 hour delay when running as a
daily scheduled task, preventing all Satellites from hitting the
Sources API simultaneously at midnight. The API endpoint passes
immediate=true to skip the delay during foremanctl deploys.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If a cloud-initiated remediation job was created for an org in the
past 24 hours, the cloud connector pipeline is proven working for
that org. Skip the Sources API call and report the org as
"Already registered" instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Show "Already registered (recent cloud remediation)" when skipping
due to recent jobs, vs "Already registered" when the Sources API
confirmed the connection exists. Also adds a test for the recent
cloud remediation skip path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jeremylenz jeremylenz force-pushed the cloud-connector-sources-announce branch from 1d6018a to f38fbf6 Compare June 12, 2026 21:14
Comment thread app/controllers/api/v2/rh_cloud/inventory_controller.rb Outdated
jeremylenz and others added 3 commits June 15, 2026 13:36
Remove "Satellite" from API endpoint description since this also
applies to Foreman.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
job_invocations table has no timestamps. Use foreman_tasks_tasks.started_at
via the :task association instead of job_invocations.created_at.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace references to the old UI button / REX job setup with the
foremanctl deploy flow and daily CloudConnectorAnnounceTask.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants